Performance: avoid over-allocation in wp_is_numeric_array()#12100
Performance: avoid over-allocation in wp_is_numeric_array()#12100dmsnell wants to merge 4 commits into
Conversation
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
|
Tested the patch locally against current Results:
The existing focused coverage for |
When a trace of allocations revealed that `wp_is_numeric_array()` accounted for a significant fraction of the allocations in a page render, it was observed that the function eagerly allocates and copies array keys and then filters them when all it wants to know is whether a single key in the array meets a condition. In this patch the `array_filter( array_keys() )` invocation is replaced with early-aborting iteration to avoid the memory allocation and copying. This patch prepared during WCEU 2026 Contributor Day. Follow-up to [34927]. Follow-up to 374b39d.
d0a5456 to
b8d0fdc
Compare
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
thanks @yusufhay — just a side note, if may not be worth your time checking those things since GitHub Actions already does in the CI jobs. @westonruter for your perusal, I don’t expect this to amount to much, but it seemed like an obvious refactor to avoid the extra allocation and computation which is immediately disregarded. on the other hand, with hundreds of recursive calls handling one final question: if we assume that my assumption is that this was the goal of the original function but the difference is in sparse arrays. if an array doesn’t start with key $a = array();
$a[1] = true;
$a[5] = false;
false === array_is_list( $a );
true === wp_is_numeric_array( $a ); |
|
@dmsnell This is great. A couple suggestions. I too have run across this function specifically when reviewing this method: I also wondered if it was originally intended to be an implementation of Maybe we should update the docblock to encourage devs to use Also related is Also related: wordpress-develop/src/wp-includes/rest-api.php Lines 1614 to 1620 in 2f682ae |
Co-Authored-By: Weston Ruter <westonruter@git.wordpress.org>
this is a really good point @westonruter, one which I think I’ve run into before and forgot.
I left a note showing the differences. Now I’m a bit worried that if we try and push people towards Perhaps in a separate PR we could examine returning this, but it’s more work than I want to devote to this right now… function wp_is_numeric_array( $data ) {
if ( ! is_array( $data ) ) {
return false;
}
if ( array_is_list( $data ) ) {
return true;
}
foreach ( $data as $key => $value ) {
if ( is_string( $key ) ) {
return false;
}
}
return true;
}the premise being that the internal speedup for most arrays would outweigh the additional calling cost. thanks for the instant review! |
| * @since 4.4.0 | ||
| * | ||
| * @param mixed $data Variable to check. | ||
| * @return bool Whether the variable is a list. |
There was a problem hiding this comment.
Adding @phpstan-assert-if-true would be a nice addition here:
| * @return bool Whether the variable is a list. | |
| * | |
| * @phpstan-assert-if-true array<int, mixed> $data |
Static analysis with PHPStan would then satisfied in cases like this when attempting to iterate over $data:
wordpress-develop/src/wp-includes/rest-api.php
Lines 966 to 968 in 2f682ae
Otherwise, PHPStan here is typing $data as mixed:
phpstan: Argument of an invalid type mixed supplied for foreach, only iterables are supported.
There was a problem hiding this comment.
Otherwise, if this were just an implementation of array_is_list() then it could do:
@phpstan-assert-if-true list<mixed> $data
But it seems perhaps unsafe to assume that every case of wp_is_numeric_array() being used to today intends array_is_list().
There was a problem hiding this comment.
@phpstan-assert-if-true array<int, mixed> $data
I have not seen this, but I guess it’s tantamount to guard functions in TypeScript? seeing as how it’s already in place elsewhere I’ll add it, though I am curious about whether you have concern about basing the types on the trust of docblock comments vs. inferences.
In TypeScript projects I’ve been skeptical of this due to the loss of constraints and soundness. It does provide good autocomplete, and a function like this should be relatively easy to ensure the constraints…
it seems perhaps unsafe to assume that every case of wp_is_numeric_array() being used to today intends array_is_list().
yeah I think so. I didn’t have confidence in my assumption about it, but I knew that changing the behavior would invite breakage. 🙃
There was a problem hiding this comment.
I have not seen this, but I guess it’s tantamount to guard functions in TypeScript? seeing as how it’s already in place elsewhere I’ll add it, though I am curious about whether you have concern about basing the types on the trust of docblock comments vs. inferences.
It's a good point. I'm not sure why PHPStan doesn't just infer what will be returned by looking at the executed code. I inquired of Claude:
Good question — and the short answer is that PHPStan deliberately doesn't do this for user-defined functions, regardless of how analyzable the body is.
PHPStan treats function calls as black boxes
When you call
wp_is_numeric_array( $x ), PHPStan only knows the declared return type:bool. It does not re-analyze the function body at each call site to figure out whattruevs.falseimplies about the arguments. So at a call site like:if ( wp_is_numeric_array( $x ) ) { // Without the annotation, $x is still `mixed` here. }…there is no narrowing at all unless you tell PHPStan the relationship. The
@phpstan-assert-if-trueannotation is precisely the mechanism for declaring "if this returnstrue, the argument isarray<int, mixed>."This is by design. Back-propagating "the return was
true, therefore argument has shape X" from arbitrary control flow is, in general, undecidable and would be prohibitively expensive to do for every function at every call site. PHPStan only does this kind of inference for:
- Hardcoded built-ins —
is_array(),is_string(),array_is_list()(which narrows tolist<...>), etc. These are special-cased in PHPStan's engine.array_is_listis exactly analogous to what we want here, but it gets narrowing only because PHPStan ships explicit knowledge of it. A WordPress function it's never heard of gets nothing.- Functions with explicit
@phpstan-assert*annotations — the manual opt-in.- Custom
TypeSpecifyingExtensions — a PHP class you register that programmatically tells PHPStan how to narrow. That's the heavier alternative to the annotation.Why the body wouldn't be "easy" to invert anyway
Even setting aside the design decision, the narrowing here isn't a simple matter of reading the body. The
trueresult comes from the fall-through path after a loop:foreach ( $data as $key => $value ) { if ( is_string( $key ) ) { return false; } } return true;To conclude
array<int, mixed>, an analyzer would have to reason: "the loop visited every key, none satisfiedis_string, array keys are only everint|string, therefore every key isint." That's universal quantification over a loop combined with exhaustiveness reasoning — exactly the sort of thing static analyzers don't attempt when deriving implications from return values.So the annotation isn't compensating for a PHPStan weakness so much as supplying knowledge PHPStan never tries to derive on its own for custom functions — the same way
@param/@returnexist because it can't always infer those either.One thing worth noting: the annotation is a promise PHPStan trusts but lightly verifies. With
@phpstan-assert*, PHPStan does check that the asserted type is compatible with what the body could return, but it won't catch a subtly-wrong assertion. So the correctness ofarray<int, mixed>here rests on the human reasoning above, not on PHPStan having proven it.
|
(Sorry, I somehow didn't submit those line review comments when I submitted my other comment.) |
Trac ticket: Core-65467
When a trace of allocations revealed that
wp_is_numeric_array()accounted for a significant fraction of the allocations in a page render, it was observed that the function eagerly allocates and copies array keys and then filters them when all it wants to know is whether a single key in the array meets a condition.In this patch the
array_filter( array_keys() )invocation is replaced with early-aborting iteration to avoid the memory allocation and copying.This patch prepared during WCEU 2026 Contributor Day.
Follow-up to [34927].
Follow-up to 374b39d.